feat: add PUT /users/:id endpoint with strict field validation - #80
Open
MariannaWay wants to merge 3 commits into
Open
feat: add PUT /users/:id endpoint with strict field validation#80MariannaWay wants to merge 3 commits into
MariannaWay wants to merge 3 commits into
Conversation
- Add updateUser(id, { name, email }) to db/store.js, following
the existing getUserById/createUser pattern
- Add PUT /:id route in routes/users.js: validates required
fields (400), returns 404 for missing users, 200 with the
updated user otherwise
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SJ3RoQf3zafWoKHJu3vqh
- Add isNonEmptyString helper: rejects arrays, numbers, booleans,
and whitespace-only strings
- Closes the gap found in review where {} or 123 as name/email
would be accepted and persisted
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SJ3RoQf3zafWoKHJu3vqh
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014SJ3RoQf3zafWoKHJu3vqh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Adds a
PUT /users/:idendpoint for updating an existing user, following the patterns already established inroutes/users.jsanddb/store.js.db/store.js— newupdateUser(id, { name, email }), written in the style ofgetUserById/createUser. It reusesgetUserByIdto locate the record, updatesnameandemail, and returns the updated user, orundefinedwhen no user matches. Validation stays out of the store, consistent with howcreateUserworks.routes/users.js— newrouter.put("/:id", ...)handler:idwithNumber(req.params.id), exactly as the existingGET /:idhandler does400 { error: "name and email are required" }when either field is missing or invalid — same status and message shape asPOST /404 { error: "User not found" }when the user doesn't exist — same shape asGET /:id200with the updated userWhy
PUT /users/:idwas the missing verb in the users resource — the API could list, fetch, and create users but not modify one.tests/update-user.test.jsspecifies the expected behavior; this makes it pass.The three commits are deliberately separate so the history shows the reasoning:
375d24fc49994ee07e857NOTES.md(plan, model choice, review findings)The review fix, in detail
The first pass validated with
if (!name || !email), mirroringPOST /. That's a truthiness check, not a type check, so these all returned200and were written to the store:An empty array is truthy, so "no value" in array form read as present. This matters more on
PUTthan onPOST: a badPOSTadds one junk record, whereas a badPUTsilently destroys a valid user's data, and the in-memory store has no persistence or history to recover from.c49994eadds anisNonEmptyStringhelper (typeof value === "string" && value.trim() !== "") and uses it in thePUTguard. All four cases above now return400with the unchanged error message.Deliberately out of scope
Noting these so reviewers know they were considered rather than missed:
Number(req.params.id)accepts/users/1e0,/users/1.0, and/users/%201%20as user1. This is inherited from the existingGET /:idhandler and is unchanged here, so it isn't a defect introduced by this PR. Worth fixing in both handlers together, or neither.{ error }JSON shape used everywhere else.server.jshas no error-handling middleware; this affects every route equally, not justPUT.POST /validation — has the same truthy-check weakness and could adoptisNonEmptyString(the helper is at module scope, ready for it), but changingPOSTis outside this task's scope." Grace "is accepted and saved with its spaces. Normalizing stored values would change store behavior beyond the validation fix.How to test
tests/update-user.test.jscovers the three specified cases: a successful update returns200with the new values, an unknown id returns404, and a missing field returns400.To exercise it by hand:
CI note
GitHub Actions doesn't run on the fork this branch came from — Actions are disabled by default on forks, so
ci.ymlwas never registered there (zero runs on any branch). Both CI steps were run locally against the pushed tree instead:npm run lintexits clean andnpm testreports 9/9 passing. CI should execute normally against this PR in the upstream repo.🤖 Generated with Claude Code
https://claude.ai/code/session_014SJ3RoQf3zafWoKHJu3vqh